[wrangler] Recognise compound statement markers without surrounding whitespace - #15226
[wrangler] Recognise compound statement markers without surrounding whitespace#15226MatheusMartinho wants to merge 6 commits into
Conversation
…hitespace The D1 SQL splitter only treated BEGIN, CASE and END as compound statement markers when they were surrounded by whitespace. SQLite also accepts them delimited by punctuation, so a trigger body ending in `INSERT ...;END;` never terminated and every following statement was swallowed into it and sent as one statement, and a trigger declared with `WHEN (1=1)BEGIN` was split in the middle of its body. Match the markers when they are delimited by any non-identifier character, keeping identifiers that merely end in a keyword, such as a `weekend` table, from matching.
🦋 Changeset detectedLatest commit: 975aa23 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
…racter Relaxing only the character before the marker made a parenthesised CASE expression, such as SUM(CASE WHEN a THEN 1 ELSE 0 END), open a compound statement that END) never closed, so the rest of the file was sent as a single statement. Accept any non-identifier character after END too, and cover the case in the tests.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
The three red
This diff only changes two regexes in the D1 SQL splitter, and the tests that cover it pass on all three platforms — on the Windows run, Worth noting when comparing against other PRs that appear fully green: they do not run the same set. Because this PR touches Happy to rebase or push an empty commit for a re-run if that helps. |
The statement scanner handled ', " and backtick quoting but not SQLite's bracket-quoted identifiers, while normalizeSqlLineEndings() in the same file already did. A ; inside such an identifier, as in CREATE TABLE metrics ([value;unit] TEXT), was treated as a statement boundary and broken fragments were sent to D1 (pre-existing, cloudflare#15228). With the punctuation-delimited markers from this branch the gap also made a [end] column inside a trigger body pop the compound statement early and chop the trigger apart. Consume [ ... ] like the other quote styles, and cover both cases in the tests.
The ASCII-only class treated a letter with a diacritical mark as a
delimiter, so a column such as néend matched the END marker and closed
a trigger body early. Match identifier characters with \p{L}\p{N}
instead, consistent with how isDollarQuoteIdentifier() in the same file
treats letters with diacritics.
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
Now rebased on main (0fde4b0), and updating my earlier note on the red checks, because the failing set has changed since I wrote it. All three fixtures jobs are green now. The two red ones are both in packages-and-tools, and neither touches D1:
This diff only changes the D1 SQL splitter in The dev-registry failure looks like the flake already being tracked on Both Devin findings are fixed and I have resolved those two threads: bracket-quoted identifiers are now consumed by Ready for a codeowner from @cloudflare/d1 or @cloudflare/wrangler whenever there is a moment. Happy to push an empty commit for a CI re-run if that helps. |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
| * include letters with diacritical marks, as in `isDollarQuoteIdentifier()`, | ||
| * so a `néend` column does not match either. | ||
| */ | ||
| const COMPOUND_STATEMENT_START = /(?<![\p{L}\p{N}_$])(BEGIN|CASE)\s$/iu; |
There was a problem hiding this comment.
🟡 Parenthesized CASE breaks trigger splitting
When a trigger contains CASE(, COMPOUND_STATEMENT_START misses the case expression. Its END closes the trigger early, splitting remaining statements apart.
Prompt for agents
Update the compound-statement start matching in packages/wrangler/src/d1/splitter.ts so BEGIN and CASE are recognized when followed by any valid non-identifier delimiter, not only whitespace. Preserve the guard against keyword suffixes in identifiers. Add a splitter regression test with a trigger containing CASE(expression) and at least one subsequent statement inside the trigger, followed by another top-level statement, so premature closure is observable.
Was this helpful? React with 👍 or 👎 to provide feedback.
| * so a `néend` column does not match either. | ||
| */ | ||
| const COMPOUND_STATEMENT_START = /(?<![\p{L}\p{N}_$])(BEGIN|CASE)\s$/iu; | ||
| const COMPOUND_STATEMENT_END = /(?<![\p{L}\p{N}_$])END[^\p{L}\p{N}_$]$/iu; |
There was a problem hiding this comment.
🟡 Decomposed identifiers break trigger splitting
With a combining accent before end, COMPOUND_STATEMENT_END mistakes part of an identifier for the terminator. Remaining trigger statements become invalid commands.
Prompt for agents
Include Unicode combining marks in the identifier character class used by both compound marker regexes in packages/wrangler/src/d1/splitter.ts. Add a regression test using an unquoted decomposed Unicode identifier whose suffix is `end` inside a multi-statement trigger, then verify the trigger and a following top-level statement remain correctly split.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Fixes #15228. Related to #15093 (see note below).
splitSqlQuery()splits a SQL file into statements forwrangler d1 execute --fileandwrangler d1 migrations apply. It tracks compound statements so that semicolons inside a trigger body do not split it, but it only recognised theBEGIN/CASE/ENDmarkers when they were surrounded by whitespace:SQLite also accepts those markers delimited by punctuation, and then the split goes wrong in two ways:
INSERT INTO audit VALUES (new.id);END;— theENDis preceded by;, so the compound statement never terminates and every following statement is swallowed into the trigger and sent to D1 as one statement. The visible symptom is silently missing result sets, not an error.CREATE TRIGGER ... WHEN (1=1)BEGIN— theBEGINis preceded by), so the compound statement never starts and the trigger body is split at its first internal semicolon.Both markers are now matched when delimited by any non-identifier character, using a negative lookbehind so identifiers that merely end in a keyword — a
weekendtable, a value of'weekend'— still do not match.Review follow-up: bracket-quoted and non-ASCII identifiers
The Devin review correctly caught that widening the delimiter class introduced two false positives, which are now fixed:
[and]are outside the identifier class, so a trigger body containing a name like[end]popped the compound statement early and the trigger was chopped apart.splitSqlIntoStatements()now consumes[…]like the other quote styles. This also fixes [wrangler] D1 SQL splitter does not treat bracket-quoted identifiers as quoted #15228, which is pre-existing: a;inside a bracket-quoted identifier (CREATE TABLE metrics ([value;unit] TEXT);) splits the statement even onmain—normalizeSqlLineEndings()in the same file already treats[…]as a quote pair, so the two scanners disagreed about the same syntax. (My earlier review reply claiming the[end]case reproduced identically onmainwas wrong — onmainthe whitespace-only markers coincidentally never fire inside[end]; I've corrected that in the thread.)éas a delimiter, so anéendcolumn matched theENDmarker. Both regexes now use Unicode identifier classes (\p{L}\p{N}_$with theuflag), consistent with howisDollarQuoteIdentifier()in the same file treats letters with diacritical marks.On #15093
That issue reports the same class of bug for a lowercase
end;. Case handling is already fixed onmain(both regexes carry/i), and I confirmed lowercase input now splits identically to uppercase, so the originally reported reproduction passes as-is. The variants above still reproduced, which is what this PR fixes.Testing
Seven regression tests in
splitter.test.ts, red/green verified:;END;,(1=1)BEGINandSUM(CASE ... END)fail againstmainand pass with this change; the false-positive guard (weekend) passes both before and after.[value;unit]fails onmainand on this branch before the scanner change (it is the [wrangler] D1 SQL splitter does not treat bracket-quoted identifiers as quoted #15228 bug), and passes now.[end]andnéendcases pass onmain, failed on this branch before the follow-up commits (the regressions Devin found), and pass again now.Full file passing;
check:typeandcheck:formatare clean. I also verified end-to-end withwrangler d1 execute --local --filethat the[value;unit]schema fails on released wrangler 4.123.0 and that the trigger examples execute correctly with this branch.Note
This contribution was written with an AI agent: Claude Code (Claude Fable 5), directed and reviewed by @MatheusMartinho, who takes responsibility for the change.